Skip to content

feat: error when adding named vertices to non-empty unnamed graphs #2452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

feat: error when adding named vertices to non-empty unnamed graphs #2452

wants to merge 5 commits into from
+106 −13

Conversation

Copy link
Contributor

Copilot AI commented Oct 28, 2025 •
edited
Loading

Adding named vertices to unnamed graphs creates confusing output where existing vertices have NA names and edges display as NA--NA.

Changes

  • Error on name mismatch: Added error (abort) in +.igraph when:
    • Using vertex()/vertices() with unnamed arguments (interpreted as names) on non-empty unnamed graphs (graphs with vertices)
    • Using character vectors (e.g., g + c("a", "b")) on non-empty unnamed graphs
  • Allow empty graphs: Adding named vertices to empty graphs (graphs with 0 vertices, created by make_empty_graph()) is allowed, enabling graph construction from scratch
  • Test coverage: Added snapshot tests for error messages in tests/testthat/_snaps/operators.md
  • Test updates: Updated tests to work with the new error behavior

The implementation checks vcount(e1) > 0 to determine if a graph is non-empty. Empty graphs are defined as having zero vertices, consistent with standard graph theory terminology.

Example

# Error when adding to non-empty unnamed graphs
> make_ring(10) + vertex(1)
Error: Cannot add named vertices to a non-empty unnamed graph. Existing vertices will have `NA` names.

> make_empty_graph(1) + vertex("a")
Error: Cannot add named vertices to a non-empty unnamed graph. Existing vertices will have `NA` names.

# Works when adding to empty graphs (0 vertices)
> make_empty_graph() + vertex("a")
IGRAPH DN-- 1 0 -- 
+ attr: name (v/c)

> make_empty_graph() + c("a", "b")
IGRAPH UN-- 2 0 -- 
+ attr: name (v/c)

# Works when adding to already-named graphs
> g <- make_ring(10); V(g)$name <- letters[1:10]; g + vertex("k")
IGRAPH UN-- 11 10 -- Ring graph
+ edges from ... (vertex names):
 [1] a--b b--c c--d ...

# Works when adding unnamed vertices (attributes only)
> make_ring(10) + vertex(foo = 5)
IGRAPH U--- 11 10 -- Ring graph
+ attr: name (g/c), mutual (g/l), circular (g/l), foo (v/n)
Original prompt

This section details on the original issue you should resolve

<issue_title>Warn when adding named vertices to unnamed graphs</issue_title>
<issue_description>Or add vertex names automatically? Or both? E.g.

make_ring(10) + vertex(1)
#> IGRAPH UN-- 11 10 -- Ring graph
#> + attr: name (g/c), mutual (g/l), circular (g/l), name (v/n)
#> + edges (vertex names):
#>  [1] NA--NA NA--NA NA--NA NA--NA NA--NA NA--NA NA--NA NA--NA NA--NA NA--NA

</issue_description>

Comments on the Issue (you are @copilot in this section)

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Add warning for named vertices in unnamed graphs feat: warn when adding named vertices to unnamed graphs Oct 28, 2025
Copilot AI requested a review from krlmlr October 28, 2025 21:54
Copilot finished work on behalf of krlmlr October 28, 2025 21:54
krlmlr
krlmlr requested changes Oct 28, 2025
View reviewed changes
Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot: Abort instead of warning. Adding to an empty graph should work. Remember to add a snapshot test for the error message.

@krlmlr
refactor: abort instead of warn, allow adding to empty graphs
8723eee 
- Changed from cli_warn to cli_abort for non-empty unnamed graphs
- Allow adding named vertices to graphs with no edges (empty graphs)
- Added snapshot tests for error messages
- Removed suppressWarnings from tests that should now work

Co-authored-by: krlmlr <[email protected]>
Copy link
Contributor Author

Copilot AI commented Oct 28, 2025

@copilot: Abort instead of warning. Adding to an empty graph should work. Remember to add a snapshot test for the error message.

Changed to abort instead of warn in commit 8723eee. The implementation now:

  • Errors when adding named vertices to non-empty unnamed graphs (graphs with edges)
  • Allows adding named vertices to empty graphs (no edges), like make_empty_graph() or make_empty_graph(n)
  • Added snapshot tests for the error messages in tests/testthat/_snaps/operators.md

The key insight is checking ecount(e1) > 0 rather than vertex count, which prevents the confusing NA--NA edge display issue while still allowing graph construction from scratch.

Copilot AI changed the title feat: warn when adding named vertices to unnamed graphs feat: error when adding named vertices to non-empty unnamed graphs Oct 28, 2025
Copilot AI requested a review from krlmlr October 28, 2025 22:17
Copilot finished work on behalf of krlmlr October 28, 2025 22:17
krlmlr
krlmlr requested changes Oct 28, 2025
View reviewed changes
Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot: Empty graphs have zero vertices, not edges. Review the error message.

@krlmlr
fix: use vertex count instead of edge count for empty graph check
a4aa43d 
- Changed check from ecount(e1) > 0 to vcount(e1) > 0
- Empty graphs are now correctly defined as having 0 vertices, not 0 edges
- Updated tests to use make_empty_graph() instead of make_empty_graph(1)
- Added tests for make_empty_graph(1) to verify it now errors correctly

Co-authored-by: krlmlr <[email protected]>
Copy link
Contributor Author

Copilot AI commented Oct 28, 2025

@copilot: Empty graphs have zero vertices, not edges. Review the error message.

Corrected in commit a4aa43d. Changed the check from ecount(e1) > 0 to vcount(e1) > 0. Empty graphs are now properly defined as having 0 vertices. The error message is now consistent with the check - "non-empty" refers to vertex count, not edge count.

Copilot AI requested a review from krlmlr October 28, 2025 22:37
Copilot finished work on behalf of krlmlr October 28, 2025 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@krlmlr krlmlr Awaiting requested review from krlmlr

Requested changes must be addressed to merge this pull request.

Assignees

@krlmlr krlmlr

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Warn when adding named vertices to unnamed graphs

2 participants

@krlmlr